fix(decisioning): TaskHandoff registry.fail/complete echo request context by bokelley · Pull Request #569 · adcontextprotocol/adcp-client-python

bokelley · 2026-05-04T11:32:33Z

Closes #563 (follow-up to #560).

The gap

PR #560 fixed context-echo on the synchronous AdcpError raise path across MCP and A2A. Background tasks (TaskHandoff) route through a separate path that #560 didn't touch: `registry.complete(task_id, result)` on success and `registry.fail(task_id, error.to_wire())` on AdcpError (or wrapped INTERNAL_ERROR). Neither echoed the request's `context` extension, so a buyer polling `tasks/get` on a deferred task lost their correlation IDs across the handoff boundary.

The fix

`_project_handoff(...)` now accepts `request_params: BaseModel | None`. Both terminal-state helpers — `_fail` and the success branch that calls `registry.complete` — apply `inject_context(_to_request_dict(request_params), wire_dict)` before passing to the registry.

The echo is now symmetric across the four terminal-state paths:

Path	Site	Status
Sync MCP / A2A success	`mcp_tools.py:2030` / `a2a_server._send_result`	already echoed (#560)
Sync AdcpError raise	`serve.py` via `build_mcp_error_result`	already echoed (#560)
Bg task success	`registry.complete` in `_project_handoff`	this PR
Bg task failure (incl. wrapped INTERNAL_ERROR)	`registry.fail` via `_fail` in `_project_handoff`	this PR

New helper

`_to_request_dict(params)` coerces a Pydantic request model (or dict) to a plain dict for `inject_context`. Empty-dict fall-through on coercion failure — the echo is a feature, not a load-bearing invariant; if Pydantic fails to dump, the wire envelope still goes out without crashing.

Tests

4 new in `test_decisioning_dispatch.py`:

success-path echo: `registry.complete` carries `context` from the request
AdcpError-path echo: `registry.fail` carries `context` alongside `adcp_error`
wrapped-INTERNAL_ERROR echo: raw `RuntimeError` → wrap → `registry.fail` envelope still echoes
no-request-params no-op: when called from test fixtures without `request_params=`, no synthesised context

42 dispatch tests green; ruff clean.

Test plan

`pytest tests/test_decisioning_dispatch.py` — 42 passed (38 existing + 4 new)
`ruff check` — clean

🤖 Generated with Claude Code

…o request context (closes #563) PR #560 fixed context-echo on the synchronous AdcpError raise path. Background tasks (TaskHandoff) route through a separate path: registry.complete on success and registry.fail on AdcpError or wrapped INTERNAL_ERROR. Neither echoed the request's context extension, so a buyer polling tasks/get on a deferred task lost their correlation IDs across the handoff boundary. _project_handoff now accepts request_params (the original Pydantic model). Both _fail and the success branch apply inject_context to the wire dict before passing to the registry. Symmetric across: - sync MCP / A2A success path (mcp_tools.py / a2a_server._send_result) - sync AdcpError path (serve.py via build_mcp_error_result) - bg task success (registry.complete) - bg task failure incl. wrapped INTERNAL_ERROR (registry.fail) A new _to_request_dict helper coerces Pydantic to dict for inject_context, with empty-dict fall-through on coercion failure. 4 new tests in test_decisioning_dispatch.py cover success-path echo, AdcpError echo, wrapped-INTERNAL_ERROR echo (RuntimeError → wrap → echo), and no-request-params no-op. 42 dispatch tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…nse top level Protocol-expert flagged the original PR put context inside result/error on the registry record. Per schemas/cache/core/tasks_get_response.json context is a top-level sibling of result/error/history. Spec-incorrect shape would have surfaced as result.context / error.context on tasks/get reads instead of top-level context. Refactor: - TaskRecord gains a request_context field; to_dict surfaces it at the top level under context key. - TaskRegistry.issue accepts request_context kwarg (additive, default None — backwards-compatible for adopters who don't thread context). - _project_handoff and _project_workflow_handoff (the second gap reviewer flagged — workflow path also issues tasks) extract context from request_params and pass to issue() once at issue-time. Terminal-state helpers (_fail, registry.complete) no longer touch context. - _to_request_dict renamed to _extract_request_context. model_dump failures now log a warning (was silent before). Tests assert top-level rec[context] (not rec[result][context] or rec[error][context]) per the spec shape. 129 tests across registry/idempotency/dispatch green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bokelley · 2026-05-04T11:51:49Z

Updated based on expert review (code-reviewer + security-reviewer + ad-tech-protocol-expert):

Spec-correctness fix (protocol-expert): Original approach injected context inside the result/error dict. Per schemas/cache/core/tasks_get_response.json, context is a top-level sibling of result/error/history. Refactored so:

TaskRecord gains a request_context field; to_dict() surfaces it at the top level under context.
TaskRegistry.issue() accepts request_context=None kwarg (additive, default backwards-compatible).
_project_handoff extracts context from request_params and passes to issue() once at issue-time. Terminal-state helpers (_fail, registry.complete) no longer mutate the wire dict.

Workflow_handoff coverage (reviewer #2): The same context-echo gap existed in _project_workflow_handoff — that path was missed in v1 of this PR. Now also threads request_params and calls issue(request_context=...).

Observability fix (security-reviewer): _to_request_dict's silent except Exception → {} is now _extract_request_context and logs a warning when model_dump fails. Buyer correlation IDs being lost are now detectable in operator logs.

Tests updated to assert top-level rec.context (not nested) and 'context' not in rec.result / rec.error to pin the spec invariant.

Open question I deferred (security-reviewer L1): credential-shaped keys in the buyer's request.context field are not currently gated by _validate_ctx_metadata_credentials. Buyer-leaks-to-themselves contract is preserved (registry is account-scoped via expected_account_id), but if tasks/get ever gains a cross-account fan-out (admin / governance read), buyer's context becomes an exfil channel. Left as a follow-up to track separately — not new in this PR.

129 tests across registry/idempotency/dispatch green; lint clean.

bokelley · 2026-05-04T11:52:57Z

Thanks for the detailed update. The spec-correctness refactor (context as top-level sibling rather than injected inside result/error), the _project_workflow_handoff coverage gap fix, and the observability improvement all look like solid improvements over the original approach. The deferred credential-leakage follow-up is a reasonable call — the account-scoping invariant holds today, and tracking it separately keeps this PR reviewable.

Ready for human review when you are.

Triaged by Claude Code. Session: https://claude.ai/code/session_01WyXp7wGvN7oX9VVcwxt9iR

Generated by Claude Code

…nit) Reviewer flagged that adding the request_context kwarg breaks custom TaskRegistry impls in the wild — Python doesn't silently absorb unknown kwargs at the call site. Adds **_extra: Any to both the Protocol declaration and InMemoryTaskRegistry.issue so the framework can introduce new optional kwargs in future versions without breaking adopters who haven't upgraded their custom impls. Custom impls MUST include **_extra: Any on their issue() signature. Documented the contract in the Protocol docstring; InMemoryTaskRegistry logs unrecognized kwargs at DEBUG so adopters notice they've fallen behind without noisy warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bokelley · 2026-05-04T13:07:56Z

Addressed the P3 nit: TaskRegistry.issue() now declares **_extra: Any (both Protocol and InMemoryTaskRegistry impl). Custom registry impls in the wild that haven't yet adopted request_context= won't TypeError when the framework introduces new optional kwargs — they silently absorb unknowns and the in-memory impl logs at DEBUG so adopters notice they've fallen behind.

Documented the forward-compat contract in the Protocol docstring: custom impls MUST include **_extra: Any on their issue() signature.

Tests still green. Ready for admin-merge.

bokelley · 2026-05-04T13:09:13Z

The **_extra: Any forward-compat contract is a clean solution — custom registry impls that lag behind the framework's kwarg additions won't TypeError, and the DEBUG log gives operators a signal without being noisy. Looks good from a triage perspective; nothing new to flag.

Triaged by Claude Code. Session: https://claude.ai/code/session_013aJ8fiv2FgJedEFoBEuHrV

Generated by Claude Code

bokelley and others added 2 commits May 4, 2026 07:31

bokelley merged commit b73b899 into main May 4, 2026
16 checks passed

github-actions Bot mentioned this pull request May 4, 2026

chore(main): release 4.4.0 #328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(decisioning): TaskHandoff registry.fail/complete echo request context#569

fix(decisioning): TaskHandoff registry.fail/complete echo request context#569
bokelley merged 3 commits intomainfrom
bokelley/issue-563-taskhandoff-context-echo

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bokelley commented May 4, 2026

The gap

The fix

New helper

Tests

Test plan

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

bokelley commented May 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant